app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)

Line range hint 666-675: Fix incomplete implementation in listReferences method

The method has several issues:

1. Missing return statement in the BRANCH case
2. Always returns an empty list regardless of the branch case
3. Incomplete implementation for tag references

Apply this fix:

@Override
public Mono<List<String>> listReferences(
        ArtifactJsonTransformationDTO artifactJsonTransformationDTO,
        Boolean checkRemoteReferences,
        RefType refType) {
    if (RefType.BRANCH.equals(refType)) {
-        listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
+        return listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
    }

    // TODO: include ref type for tags in fsGit Handler
    return Mono.just(List.of());
}

app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)

361-374: Check for message usage when tagging.
You are correctly setting the tag message. Verify that you handle empty or null messages gracefully, if needed.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)

537-546: Handle unclean status more gracefully.
The error message is generic. If possible, provide details as to why the status was unclean for faster troubleshooting.

---

568-571: Verify publish logic post reference creation.
Publishing can sometimes be asynchronous or partial. Double-check that the artifact is in a stable state if post-publish fails.

app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitArtifactHelperCE.java (1)

72-73: Publish logic after reference creation.
This added method is straightforward. Confirm if you need to handle any exceptions within the publication flow for newly created references.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)

367-393: Consider enhancing error handling in createAndCheckoutReference.

The implementation looks good, but consider adding specific error handling for common Git operations failures.

 @Override
 public Mono<String> createAndCheckoutReference(Path repoSuffix, GitRefDTO gitRefDTO) {
     RefType refType = gitRefDTO.getRefType();
     String refName = gitRefDTO.getRefName();

     return Mono.using(
             () -> Git.open(createRepoPath(repoSuffix).toFile()),
             git -> Mono.fromCallable(() -> {
                     log.info(
                             "{} : Creating reference of type {} and name {} for the repo {}",
                             Thread.currentThread().getName(),
                             refType.name(),
                             refName,
                             repoSuffix);

                     if (RefType.TAG.equals(refType)) {
                         return createTag(git, gitRefDTO);
                     }

                     return createAndCheckoutBranch(git, gitRefDTO);
                 })
+                .onErrorResume(GitAPIException.class, e -> {
+                    log.error("Git operation failed: {}", e.getMessage());
+                    return Mono.error(new Exception("Failed to create reference: " + e.getMessage()));
+                })
                 .timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS))
                 .name(GitSpan.FS_CREATE_BRANCH)
                 .tap(Micrometer.observation(observationRegistry)),
             Git::close)
     .subscribeOn(scheduler);
 }

---

375-381: Enhance logging for better observability.

Consider adding debug logs for operation success/failure states.

 log.info(
         "{} : Creating reference of type {} and name {} for the repo {}",
         Thread.currentThread().getName(),
         refType.name(),
         refName,
         repoSuffix);
+log.debug("Starting {} operation for reference {}", refType, refName);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)

692-695: Tie lock release failures to additional logging.
Releasing file locks on error is an excellent design choice. However, log the result of releaseLockMono for better visibility in case of lock-release failures.

---

697-700: Provide user-friendly instructions for current-branch deletions.
The exception message is correct. You might enhance user guidance (e.g., specify a different branch first) so they know how to resolve this situation.

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)

422-429: Consider adding error handling for checkout failures

The checkout operation could fail for various reasons (e.g., conflicts, network issues). Consider adding specific error handling to provide better feedback to users.

 checkedOutArtifactMono = gitHandlingService
         .checkoutArtifact(jsonTransformationDTO)
+        .onErrorResume(error -> {
+            log.error("Error during checkout: {}", error.getMessage());
+            return Mono.error(new AppsmithException(
+                AppsmithError.GIT_ACTION_FAILED,
+                "checkout",
+                error.getMessage()));
+        })
         .flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(

---

Line range hint 529-550: Enhance validation logic with early returns

The validation and preparation logic is well-structured but could be more concise with early returns.

-return refCreationValidationMono.flatMap(isOkayToProceed -> {
-    if (!TRUE.equals(isOkayToProceed)) {
-        return Mono.error(new AppsmithException(
-                AppsmithError.GIT_ACTION_FAILED,
-                "ref creation",
-                "status unclean"));
-    }
-    return Mono.zip(newArtifactFromSourceMono, artifactExchangeJsonMono);
-});
+return refCreationValidationMono
+    .filter(TRUE::equals)
+    .switchIfEmpty(Mono.error(new AppsmithException(
+        AppsmithError.GIT_ACTION_FAILED,
+        "ref creation",
+        "status unclean")))
+    .then(Mono.zip(newArtifactFromSourceMono, artifactExchangeJsonMono));

---

572-577: Consider adding transaction boundary

The post-reference creation operations (publish and discard) should ideally be atomic. Consider wrapping them in a transaction.

---

1424-1432: Consider adding timeout for Git status operations

Git status operations can be time-consuming. Consider adding a timeout to prevent long-running operations.

 return Mono.zip(prepareForStatus, fetchRemoteMono).flatMap(tuple2 -> {
     return gitHandlingService
             .getStatus(jsonTransformationDTO)
+            .timeout(Duration.ofSeconds(30))
+            .onErrorResume(TimeoutException.class, ex -> 
+                Mono.error(new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT)))
             .flatMap(gitStatusDTO -> {
                 return gitRedisUtils
                         .releaseFileLock(baseArtifactId, isFileLock)
                         .thenReturn(gitStatusDTO);
             });
 });

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)

1219-1226: Consider handling specific Git commit errors

The error handling could be more granular to provide better user feedback for different failure scenarios.

-                                return gitRedisUtils.releaseFileLock(baseArtifact.getId(), TRUE)
-                                    .then(gitAnalyticsUtils
+                                String errorMessage = error.getMessage();
+                                if (error instanceof org.eclipse.jgit.api.errors.EmptyCommitException) {
+                                    errorMessage = "No changes to commit";
+                                } else if (error instanceof org.eclipse.jgit.api.errors.UnmergedPathsException) {
+                                    errorMessage = "Please resolve conflicts before committing";
+                                }
+                                return gitRedisUtils.releaseFileLock(baseArtifact.getId(), TRUE)
+                                    .then(gitAnalyticsUtils

---

1696-1699: Enhance error messages in discardChanges operation

While the error handling is good, the error messages could be more descriptive to help users understand and resolve issues.

-                            return Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout"));
+                            String errorContext = error instanceof org.eclipse.jgit.api.errors.CheckoutConflictException
+                                    ? "Unable to discard changes due to conflicts"
+                                    : "Failed to discard changes";
+                            return Mono.error(new AppsmithException(
+                                    AppsmithError.GIT_ACTION_FAILED,
+                                    "discard",
+                                    errorContext + ": " + error.getMessage()));

Also applies to: 1701-1757

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro